Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add task solution #983

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

add task solution #983

wants to merge 8 commits into from

Conversation

1luki9901
Copy link

- [DEMO LINK](https://1luki9901.github.io/js_2048_game/)

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing a great job! 😊 However, there are a few areas that need some improvement for the code to be more efficient and user-friendly. Please consider the feedback and make the necessary changes. Keep up the good work! 👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

const Game = require('../modules/Game.class');

// eslint-disable-next-line no-unused-expressions
('use strict');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'use strict' directive is usually placed at the beginning of a script, it's not working after the first executable statement.

const Game = require('../modules/Game.class');

// eslint-disable-next-line no-unused-expressions
('use strict');

// Uncomment the next lines to use your game instance in the browser
// const Game = require('../modules/Game.class');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commented out line of code seems to be unnecessary since the Game class is already imported at the beginning of the file. It's a good practice to remove commented out code to keep your codebase clean.

Comment on lines 20 to 21
const state = game.getState();
const cells = gameBoard.getElementsByClassName('field-cell');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally a good practice to cache DOM elements that you'll need to reference multiple times. Consider storing the 'cells' collection in a variable outside of the 'updateUI' function. This way, you won't need to query the DOM every time 'updateUI' is called, which can improve performance.

Comment on lines 40 to 48
startButton.addEventListener('click', () => {
if (game.getStatus() === 'playing') {
game.restart();
} else {
game.start();
}
updateUI();
startButton.textContent = 'Restart';
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The click event listener on the 'startButton' is changing the button's text content to 'Restart' every time it's clicked. This could potentially be confusing to the user if the game is already in progress. Consider updating the button's text context based on the game's status.

Comment on lines 51 to 72
document.addEventListener('keydown', (event) => {
let moved = false;

switch (event.key) {
case 'ArrowLeft':
moved = game.moveLeft();
break;
case 'ArrowRight':
moved = game.moveRight();
break;
case 'ArrowUp':
moved = game.moveUp();
break;
case 'ArrowDown':
moved = game.moveDown();
break;
}

if (moved) {
updateUI();
}
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'keydown' event listener is currently set on the 'document'. This means that any key press will trigger this event, even when the user is not focused on the game. Consider setting the 'keydown' event listener on a specific element related to the game, to ensure that the game controls only function when the game is in focus.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job so far! 😊 There are a few areas that need improvement for better performance and readability. Please consider the provided feedback and make the necessary changes. Keep going! 💪

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

// const Game = require('../modules/Game.class');

// eslint-disable-next-line no-unused-expressions
('use strict');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'use strict' directive is not needed here since it's already inside a module. Modules are always in strict mode.

Comment on lines 20 to 38
function updateUI() {
const state = game.getState();

for (let i = 0; i < cells.length; i++) {
const cell = cells[i];
const value = state[Math.floor(i / 4)][i % 4];

cell.className = 'field-cell';

if (value) {
cell.classList.add(`field-cell--${value}`);
cell.textContent = value;
} else {
cell.textContent = '';
}
}
scoreElement.textContent = game.getScore();
statusElement.textContent = game.getStatus();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This updateUI function seems to be doing a lot of work. It might be more efficient to only update the parts of the UI that have changed, rather than updating the entire UI every time.

Comment on lines 53 to 74
gameBoard.addEventListener('keydown', (event) => {
let moved = false;

switch (event.key) {
case 'ArrowLeft':
moved = game.moveLeft();
break;
case 'ArrowRight':
moved = game.moveRight();
break;
case 'ArrowUp':
moved = game.moveUp();
break;
case 'ArrowDown':
moved = game.moveDown();
break;
}

if (moved) {
updateUI();
}
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This event listener for keydown events on the gameBoard element might not work as expected. The keydown event is typically listened for on the document or window object, not on specific elements, unless those elements are focused. You may want to reconsider where you're attaching this event listener.

Comment on lines 56 to 68
switch (event.key) {
case 'ArrowLeft':
moved = game.moveLeft();
break;
case 'ArrowRight':
moved = game.moveRight();
break;
case 'ArrowUp':
moved = game.moveUp();
break;
case 'ArrowDown':
moved = game.moveDown();
break;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch-case statement here could be simplified by using an object mapping instead. This would map each key to a corresponding function on the game object, reducing the amount of code and improving readability.

Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Demo link doesn't work.

@@ -1,7 +1,69 @@
'use strict';
// const Game = require('../modules/Game.class');

// Uncomment the next lines to use your game instance in the browser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is that file?

@1luki9901
Copy link
Author

Monosnap add task solution by 1luki9901 · Pull Req
Monosnap README md - js_2048_game - Visual Studio

Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link you provided doesn't work. I noticed that the Game.class file is present, but there haven't been any changes made to it. You need to implement all the logic within that file to make the game functional.

@1luki9901 1luki9901 closed this Oct 17, 2024
@1luki9901 1luki9901 deleted the develop branch October 17, 2024 19:33
@1luki9901 1luki9901 restored the develop branch October 17, 2024 19:38
@1luki9901 1luki9901 reopened this Oct 17, 2024
Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rerequest

@1luki9901 1luki9901 requested a review from Zibi95 October 22, 2024 17:38
Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey. Good job with implementation, but I need working DEMO link so I can check if everything is working. You have to run npm run deploy in the terminal.

@1luki9901
Copy link
Author

Monosnap README md - js_2048_game - Visual Studio
npm run deploy nie działa

@1luki9901 1luki9901 requested a review from Zibi95 October 24, 2024 11:04
Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's resolve the problem on the Mate Academy chat, and once you successfully deploy, you can request a review again.

Copy link

@Zibi95 Zibi95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Game doesn't work at all. After clicking start button nothing happens.

@1luki9901
Copy link
Author

2048.-.Google.Chrome.2024-10-26.19-09-42.mp4

No właśnie nie ogarniam czemu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants